tests: assert histogram/exp_histogram temporality survives in_opentelemetry -> out_opentelemetry round trip#12042
Open
truongnht wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe OpenTelemetry integration test now verifies that histogram aggregation temporality remains ChangesHistogram temporality validation
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
before the fix 
patch fluent-operator with the fix image 
|
…emetry -> out_opentelemetry round trip The fix itself (fluent/cmetrics#279) is already included via the lib: cmetrics: upgrade to v2.2.0 bump on master. tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py::test_opentelemetry_to_opentelemetry_histogram_and_gauge_metrics already sends a histogram with aggregation_temporality=1 (DELTA) through a real in_opentelemetry -> out_opentelemetry pipeline (test_metrics_002.in.json), but never asserted on the output's aggregationTemporality field. That gap is exactly how the cmetrics bug (histograms/exp_histograms losing their OTLP temporality across Fluent Bit's internal msgpack buffer) went unnoticed. Adds the missing assertion so a regression here is caught going forward. Verified locally against both sides of the cmetrics fix: - Built Fluent Bit with lib/cmetrics reverted to pre-v2.2.0 (unpatched): this test fails - the response's histogram has no aggregationTemporality field at all, since UNSPECIFIED is omitted by proto JSON serialization. - Built Fluent Bit with lib/cmetrics at v2.2.0 (patched): this test passes, with aggregationTemporality: AGGREGATION_TEMPORALITY_DELTA correctly present in the OTLP output. - Full in_opentelemetry (62 tests) and out_opentelemetry (17 tests) suites pass with no regressions. Also reproduced end-to-end against a real cluster: ran a k6 OTLP load test through in_opentelemetry -> out_opentelemetry -> Prometheus with the unpatched image (k6's histogram metrics were rejected by Prometheus with "invalid temporality and type combination") and again with a patched image (same metrics landed correctly, confirmed via direct Prometheus API query). Signed-off-by: Truong Nguyen <truong.nguyen_1@philips.com>
truongnht
force-pushed
the
fix/histogram-temporality-msgpack
branch
from
f0eb9d9 to
a7835d8
Compare
truongnht
changed the title
Author
|
Rebased onto master — the cmetrics fix landed via the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The actual fix (fluent/cmetrics#279) is already included via the
lib: cmetrics: upgrade to v2.2.0bump merged into master. This PR is now just the missing test coverage.tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py::test_opentelemetry_to_opentelemetry_histogram_and_gauge_metricsalready sends a histogram withaggregation_temporality: 1(DELTA) through a realin_opentelemetry → out_opentelemetrypipeline (test_metrics_002.in.json), but never asserted on the output'saggregationTemporalityfield. That gap is exactly how the cmetrics bug — histograms/exp_histograms losing their OTLP temporality across Fluent Bit's internal msgpack buffer — went unnoticed. This adds the missing assertion so a regression here is caught going forward.Background
Original bug:
pack_header()in cmetrics'cmt_encode_msgpack.conly wrote theaggregation_typemeta field forCMT_COUNTER. Histograms and exp_histograms decoded from OTLP byin_opentelemetry, buffered internally as msgpack, and re-encoded to OTLP byout_opentelemetrylost their temporality in that round trip. Downstream OTLP receivers that validate temporality (e.g. Prometheus withotlp-deltatocumulativeenabled) rejected the metric withinvalid temporality and type combination. Filed as #278, fixed in fluent/cmetrics#279.Test plan
lib/cmetricsreverted to pre-v2.2.0 (unpatched): confirmedtest_opentelemetry_to_opentelemetry_histogram_and_gauge_metricsfails — the response's histogram has noaggregationTemporalityfield at all (proto JSON omitsUNSPECIFIED)lib/cmetricsat v2.2.0 (patched): confirmed the same test passes, withaggregationTemporality: AGGREGATION_TEMPORALITY_DELTAcorrectly present in the OTLP outputin_opentelemetryintegration suite (62 tests) — all pass, no regressionsout_opentelemetryintegration suite (17 tests) — all pass, no regressionsin_opentelemetry → out_opentelemetry → Prometheuswith the unpatched image (k6's histogram metrics rejected by Prometheus) and again with a patched image (same metrics landed correctly, confirmed via direct Prometheus API query)Summary by CodeRabbit